Skip to content

fix: add boundary checks for u64 shift/rotate operations - #3368

Open
Sertug17 wants to merge 4 commits into
0xMiden:nextfrom
Sertug17:fix/u64-boundary-checks
Open

fix: add boundary checks for u64 shift/rotate operations#3368
Sertug17 wants to merge 4 commits into
0xMiden:nextfrom
Sertug17:fix/u64-boundary-checks

Conversation

@Sertug17

@Sertug17 Sertug17 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #3360

Add dup u32lt.64 assert.err boundary checks to:

  • u64::shl
  • u64::shr
  • u64::rotl
  • u64::rotr

These procedures documented that shift/rotation amounts must be in [0, 64), but lacked runtime enforcement. This matches the existing pattern in u128::shr.

Includes tests for n = 64 and n = 100 for each procedure, as requested by @huitseeker.

@github-actions

github-actions Bot commented Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown

Automated check (CONTRIBUTING.md)

Findings:

  • Add a short Rationale that explains why the change is needed.

Next steps:

@Sertug17 Sertug17 mentioned this pull request Jul 13, 2026
Open
@Sertug17

Sertug17 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Rationale

The u64::shr, u64::shl, u64::rotl, and u64::rotr procedures in u64.masm documented that shift/rotation amounts must be in [0, 64), but this bound was not enforced at runtime. Passing n >= 64 produces silent data corruption:

  • shr: pow2 overflow → incorrect u32divmod results
  • shl: wrapping_mul with 2^n wraps modulo 2^64 → zero/invalid output
  • rotl/rotr: rotation logic crosses 32-bit limb boundaries incorrectly

This fix adds dup u32lt.64 assert.err checks (matching u128::shr pattern) to enforce the documented contract and prevent undefined behavior.

Test Plan

All tests in crates/lib/core/tests/math/u64_mod.rs:

cargo test -p miden-core-lib shr_out_of_range_errors
cargo test -p miden-core-lib shl_out_of_range_errors
cargo test -p miden-core-lib rotl_out_of_range_errors
cargo test -p miden-core-lib rotr_out_of_range_errors

@Sertug17

Sertug17 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

/quality-review

huitseeker
huitseeker requested changes Jul 15, 2026
View reviewed changes

@huitseeker huitseeker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good overall. Please update the cycle counts and fix CI.

Comment thread crates/lib/core/asm/math/u64.masm
#! [n, a_lo, a_hi, ...] -> [c_lo, c_hi, ...], where c = (a << n) mod 2^64.
#! This takes 21 cycles.
pub proc shl(n: u32, a: u64) -> u64
dup u32lt.64 assert.err="shift amount must be in the range [0, 64)"

@huitseeker huitseeker Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds work before the old body, so the cycle-count docs above this and the other three changed procs now look stale. Could you recompute and update the This takes ... cycles comments for shl, shr, rotl, and rotr?

One way to check them is to add a temporary core-lib test that wraps each exec.u64::* call with clk before and after the call, then computes the delta. Since clk itself costs one cycle, subtract the first clk instruction from the measured difference.

@Sertug17
Sertug17 force-pushed the fix/u64-boundary-checks branch from 77c6e02 to c144716 Compare July 15, 2026 19:44
@Sertug17

Sertug17 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

@huitseeker Review comments addressed:

  1. Cycle counts updated: shl 21→25, shr 60/61→64/65, rotl 46→50, rotr 60→64 (+4 cycles each for dup u32lt.64 assert.err).
  2. CHANGELOG: Entry added under v0.26.0 (unreleased).
  3. Rebased on latest next.

PTAL!

huitseeker
huitseeker reviewed Jul 17, 2026
View reviewed changes

@huitseeker huitseeker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need a rebase on the latest origin/next, beware that the Changelog format has changed a bit

Comment thread crates/lib/core/asm/math/u64.masm Outdated
#! Stack transition looks as follows:
#! [n, a_lo, a_hi, ...] -> [c_lo, c_hi, ...], where c = (a << n) mod 2^64.
#! This takes 21 cycles.
#! This takes 25 cycles.

@huitseeker huitseeker Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these cycle counts are still low. The added check costs more than 4 cycles: u32lt.64 with an immediate is 4 cycles by itself, and the surrounding dup plus assert.err add 2 more.

Could you recompute these with a temporary clk-wrapped core-lib test before updating the comments? That should catch shl, shr, rotl, and rotr without having to count the expanded instructions by hand.

@Sertug17
Sertug17 force-pushed the fix/u64-boundary-checks branch from c144716 to 30464ea Compare July 17, 2026 19:37
@Sertug17

Sertug17 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

@huitseeker Rebased on latest next. PTAL!

@Sertug17
Sertug17 force-pushed the fix/u64-boundary-checks branch 2 times, most recently from 5770c06 to 82de925 Compare July 17, 2026 20:09
@Sertug17

Sertug17 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

You're right I recounted: dup (1) + push.64 (1) + u32lt (3) + assert.err (1) = +6 cycles, not +4. Updated: shl 21→27, shr 60/61→66/67, rotl 46→52, rotr 60→66. Also fixed CHANGELOG placement under #### Changes and rebased on latest next. PTAL!

@Sertug17
Sertug17 force-pushed the fix/u64-boundary-checks branch from 82de925 to f0c7580 Compare July 18, 2026 06:51
@Sertug17

Sertug17 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Updated generated core library docs (crates/lib/core/docs/) to reflect the new cycle counts. CI check core library docs should pass now.

@Sertug17
Sertug17 force-pushed the fix/u64-boundary-checks branch from cca1b5f to d6e075c Compare July 23, 2026 20:17
@Sertug17
Sertug17 force-pushed the fix/u64-boundary-checks branch from d6e075c to 6ebe174 Compare August 4, 2026 20:55
@Sertug17

Sertug17 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@huitseeker Rebased on latest next and resolved CHANGELOG conflicts. All cycle counts and docs are updated. PTAL!

@Sertug17
Sertug17 force-pushed the fix/u64-boundary-checks branch from 6ebe174 to 6abe8aa Compare August 6, 2026 11:39
@Sertug17

Sertug17 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on latest next branch is up to date, all checks were passing on the previous push. @huitseeker ready for re-review when you get a chance.

@Sertug17

Sertug17 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

@huitseeker I've added a clk-based cycle count regression test (shift_rotate_cycle_baselines) that empirically measures the cycle delta for each shift/rotate operation, following the same pattern used in hash_precompile_cycle_baselines.

The test uses clk + mem_store/mem_load to capture the cycle counter before and after each exec.u64::* call, then asserts against known baselines:

  • shl (n=5): 51 cycles
  • shr (n=5): 67 cycles
  • shr (n=33): 67 cycles
  • rotl (n=5): 51 cycles
  • rotr (n=5): 67 cycles

The boundary check (dup u32lt.64 assert.err) adds a consistent +6 cycles to each procedure body, matching the hand-counted instruction costs: dup(1) + push.64(1) + u32lt(3) + assert.err(1).

If any future change alters the cycle count, this test will catch it and report the exact mismatch.

@Sertug17

Sertug17 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Have you had a chance to examine this, Sir? I made the necessary changes as requested. @huitseeker

@Sertug17
fix: add boundary checks for u64 shift/rotate operations
d822369 
Signed-off-by: Sertug17 <104278804+Sertug17@users.noreply.github.com>
@Sertug17
Add clk-based cycle count regression tests for u64 shift/rotate ops
a93f5df 
Replace individual measure_* tests with a single shift_rotate_cycle_baselines
test following the project's established pattern (hash_precompile_cycle_baselines).
Uses clk + mem_store/mem_load to measure cycle deltas and asserts against known
baselines to catch unintended cycle count changes.
@Sertug17
Sertug17 force-pushed the fix/u64-boundary-checks branch from dc08da9 to a93f5df Compare August 15, 2026 17:27
@Sertug17

Sertug17 commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

@huitseeker Rebased on latest next. All checks were passing before ready for re-review when you get a chance.

Al-Kindi-0
Al-Kindi-0 requested changes Aug 19, 2026
View reviewed changes

@Al-Kindi-0 Al-Kindi-0 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In CHANGELOG.md under the v0.29.0 Changes section, this PR adds the #3371 entry a second time and records #3368 under the already released v0.29.0, even though this PR is still open. Please remove the duplicate #3371 line and move the #3368 entry to the v0.30.0 (Unreleased) Fixes section.

@Sertug17

Sertug17 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Okay. I'll do it as soon as possible. @Al-Kindi-0

@Sertug17

Sertug17 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

@Al-Kindi-0 Done removed the duplicate #3371 entry and moved the #3368 entry from v0.29.0 to the v0.30.0 (Unreleased) Fixes section. @huitseeker

Al-Kindi-0
Al-Kindi-0 approved these changes Aug 19, 2026
View reviewed changes

@Al-Kindi-0 Al-Kindi-0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tyvm

@Sertug17

Sertug17 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Thank you @Al-Kindi-0 . Did you have a chance to check @huitseeker ?

@Sertug17

Sertug17 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

This project has been on hold for a very long time. Would you be able to review it again? @huitseeker

@huitseeker huitseeker added this to the v0.31.0 milestone Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@huitseeker huitseeker huitseeker requested changes
@Al-Kindi-0 Al-Kindi-0 Al-Kindi-0 approved these changes
@Nashtare Nashtare Awaiting requested review from Nashtare

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

quality-concern

Projects

None yet

Milestone

v0.31.0

Development

Successfully merging this pull request may close these issues.

u64::shr, u64::shl, and u64::rotl missing boundary checks for shift/rotation amount

3 participants

@Sertug17 @huitseeker @Al-Kindi-0